hack/verify-staging-imports.sh: print actual dependencies #40659

sttts · 2017-01-29T12:30:46Z

Example:

$ hack/verify-staging-imports.sh
apiserver has a forbidden dependency:

  k8s.io/apiserver/pkg/admission imports k8s.io/apimachinery/pkg/api/errors
  k8s.io/apiserver/pkg/admission imports k8s.io/apimachinery/pkg/api/meta
  k8s.io/apiserver/pkg/admission imports k8s.io/apimachinery/pkg/runtime
  k8s.io/apiserver/pkg/admission imports k8s.io/apimachinery/pkg/runtime/schema
  k8s.io/apiserver/pkg/admission imports k8s.io/apimachinery/pkg/util/errors
  k8s.io/apiserver/pkg/admission imports k8s.io/apimachinery/pkg/util/sets

k8s-github-robot · 2017-01-29T12:30:51Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

Needs approval from an approver in each of these OWNERS Files:

hack/OWNERS

We suggest the following people:
cc @zmerlynn
You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-reviewable · 2017-01-29T12:31:51Z

This change is

sttts · 2017-01-29T18:27:18Z

@k8s-bot unit test this

deads2k · 2017-01-30T12:47:00Z

hack/verify-staging-imports.sh

@@ -25,8 +25,20 @@ kube::golang::setup_env


 for dep in $(ls -1 ${KUBE_ROOT}/staging/src/k8s.io/); do
-	if go list -f {{.Deps}} ./vendor/k8s.io/${dep}/... | tr " " '\n' | grep k8s.io/kubernetes | grep -v 'k8s.io/kubernetes/vendor' | LC_ALL=C sort -u | grep -e "."; then
-		echo "${dep} has a cyclical dependency"
+	if go list -f "{{.Deps}}" ./vendor/k8s.io/${dep}/... | tr " " '\n' | grep k8s.io/kubernetes | grep -v 'k8s.io/kubernetes/vendor' | LC_ALL=C sort -u | grep -qe "."; then


Is there some way to extract the printing logic so we can reasonably prevent client-go from depending on apiserver without copy/pasting this monster?

certainly this can be abstracted.

certainly this can be abstracted.

Ok, I'm bash challenged. You mind setting it up and then this lgtm.

deads2k · 2017-01-30T12:48:48Z

hack/verify-staging-imports.sh

+	if go list -f "{{.Deps}}" ./vendor/k8s.io/${dep}/... | tr " " '\n' | grep k8s.io/kubernetes | grep -v 'k8s.io/kubernetes/vendor' | LC_ALL=C sort -u | grep -qe "."; then
+		echo "${dep} has a cyclical dependency:"
+		echo
+		go list -f '{{.ImportPath}} {{.Deps}}' ./vendor/k8s.io/${dep}/... |


@liggitt @stevekuznetsov @smarterclayton @ncdc I feel like this deserves some honorable mention or something for most sed pipes I've been in our bash.

Surely there is a cleaner and more approachable way to do this using the Golang AST API... @deads2k you owe me lunch for making this the first thing I looked at this morning

deads2k · 2017-01-30T12:49:27Z

hack/verify-staging-imports.sh

+		go list -f '{{.ImportPath}} {{.Deps}}' ./vendor/k8s.io/${dep}/... |
+			sed 's|^k8s.io/kubernetes/vendor/\([-a-zA-Z./0-9_]*\)|\1:|' | # remove vendor prefix of ImportPath
+			sed 's| k8s.io/kubernetes/vendor/[-a-zA-Z./0-9_]*||g' |       # remove vendored packages
+			grep k8s.io/kubernetes |                                      # only show packages with k8s.io/kubernetes deps


if you put in newlines laters, how is grep working right?

if one k8s.io/kubernetes is found it's enough to consider the whole line. If not, ignore the whole line.

deads2k · 2017-01-30T12:53:39Z

I'm confident it still exits 1 when it fails. I'm unsure about the pipe monster and a little concerned about finding internal cycles. Is there an easy way to make it workable for client-go to apiserver style deps?

sttts · 2017-01-30T15:00:06Z

Updated, still WIP. Stepping out for an hour, then testing.

sttts · 2017-01-30T16:01:13Z

@deads2k @stevekuznetsov ptal. #40688 is supposed to fail.

stevekuznetsov · 2017-01-30T16:06:42Z

I still don't understand why this is a Bash script. The Go team exposes a great library for programatically working with Go source. Writing a short Go script using ast would be simple, would not have worries about return codes, and would not need ugly sed expressions, worries about compatibility ...

sttts · 2017-01-30T16:09:51Z

@stevekuznetsov feel free to rewrite it in Go as a follow-up. IMO it's not really important for this tiny script. We have bash code (look into Makefile.generated_files for instance) which is 3 orders of magnitude more awful and worth the energy spent for a rewrite.

deads2k · 2017-01-30T16:16:52Z

hack/verify-staging-imports.sh

-	fi
-done
+RC=0
+print_forbidden_imports apimachinery k8s.io/ || RC=1


so this ended up a lot better.

If at any point you expect set -o errexit to alter the flow of commands inside of print_forbidden_imports -- like, a failure somewhere in there you would expect to exit early ... in this construct, it would not. function_call || something will turn off errexit inside the function and there is no way to turn it back on.

good point. thanks. Those details of shell code...

deads2k · 2017-01-30T16:17:47Z

hack/verify-staging-imports.sh

+    shift
+    local RE=""
+    local SEP=""
+    for CLAUSE in "$@"; do


Why doesn't this pick up the first argument?

shift above removes it

Because of the shift on line 28

deads2k · 2017-01-30T16:20:26Z

lgetm.

deads2k · 2017-01-30T16:21:09Z

@eparis don't freak out this time. I'm going to open a pull to add myself to the list

eparis · 2017-01-30T16:23:57Z

@deads2k RAWR!

k8s-github-robot · 2017-01-31T01:14:55Z

Automatic merge from submit-queue (batch tested with PRs 40703, 40093, 40618, 40659, 39810)

sttts added the release-note-none Denotes a PR that doesn't merit a release note. label Jan 29, 2017

sttts assigned deads2k Jan 29, 2017

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 29, 2017

k8s-github-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jan 29, 2017

deads2k reviewed Jan 30, 2017

View reviewed changes

sttts force-pushed the sttts-print-staging-import-cycles branch from 49642ac to 3e58f7c Compare January 30, 2017 14:58

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 30, 2017

sttts changed the title ~~hack/verify-staging-imports.sh: print actual dependencies~~ [WIP] hack/verify-staging-imports.sh: print actual dependencies Jan 30, 2017

sttts force-pushed the sttts-print-staging-import-cycles branch 2 times, most recently from 72c26d6 to ea79fe5 Compare January 30, 2017 15:48

sttts changed the title ~~[WIP] hack/verify-staging-imports.sh: print actual dependencies~~ hack/verify-staging-imports.sh: print actual dependencies Jan 30, 2017

sttts force-pushed the sttts-print-staging-import-cycles branch from ea79fe5 to 78e7e51 Compare January 30, 2017 15:58

sttts mentioned this pull request Jan 30, 2017

[FAIL] hack/verify-staging-imports.sh #40688

Closed

deads2k reviewed Jan 30, 2017

View reviewed changes

deads2k added approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jan 30, 2017

hack/verify-staging-imports.sh: print forbidden dependencies

22e9db0

sttts force-pushed the sttts-print-staging-import-cycles branch from 78e7e51 to 22e9db0 Compare January 30, 2017 16:26

k8s-github-robot assigned spxtr and unassigned deads2k Jan 30, 2017

spxtr assigned deads2k and unassigned spxtr Jan 30, 2017

k8s-github-robot assigned jbeda and unassigned deads2k Jan 30, 2017

apelisse assigned deads2k Jan 31, 2017

k8s-github-robot assigned zmerlynn Jan 31, 2017

k8s-github-robot merged commit 2975475 into kubernetes:master Jan 31, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

hack/verify-staging-imports.sh: print actual dependencies #40659

hack/verify-staging-imports.sh: print actual dependencies #40659

sttts commented Jan 29, 2017 •

edited

k8s-github-robot commented Jan 29, 2017

k8s-reviewable commented Jan 29, 2017

sttts commented Jan 29, 2017

deads2k Jan 30, 2017

sttts Jan 30, 2017

deads2k Jan 30, 2017

deads2k Jan 30, 2017

stevekuznetsov Jan 30, 2017

deads2k Jan 30, 2017

sttts Jan 30, 2017

deads2k commented Jan 30, 2017

sttts commented Jan 30, 2017

sttts commented Jan 30, 2017

stevekuznetsov commented Jan 30, 2017

sttts commented Jan 30, 2017

deads2k Jan 30, 2017

stevekuznetsov Jan 30, 2017

sttts Jan 30, 2017

deads2k Jan 30, 2017

stevekuznetsov Jan 30, 2017

ncdc Jan 30, 2017

deads2k commented Jan 30, 2017

deads2k commented Jan 30, 2017

eparis commented Jan 30, 2017 •

edited by apelisse

k8s-github-robot commented Jan 31, 2017

hack/verify-staging-imports.sh: print actual dependencies #40659

hack/verify-staging-imports.sh: print actual dependencies #40659

Conversation

sttts commented Jan 29, 2017 • edited

k8s-github-robot commented Jan 29, 2017

k8s-reviewable commented Jan 29, 2017

sttts commented Jan 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Jan 30, 2017

sttts commented Jan 30, 2017

sttts commented Jan 30, 2017

stevekuznetsov commented Jan 30, 2017

sttts commented Jan 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Jan 30, 2017

deads2k commented Jan 30, 2017

eparis commented Jan 30, 2017 • edited by apelisse

k8s-github-robot commented Jan 31, 2017

sttts commented Jan 29, 2017 •

edited

eparis commented Jan 30, 2017 •

edited by apelisse